Conversation
Signed-off-by: Olasoji <olasoji.denloye@intel.com>
Signed-off-by: Olasoji <olasoji.denloye@intel.com>
Signed-off-by: Olasoji <olasoji.denloye@intel.com>
Signed-off-by: Olasoji <olasoji.denloye@intel.com>
Signed-off-by: Olasoji <olasoji.denloye@intel.com>
- Added explicit end-of-stream signaling for IGZIP inflate via out-param and wired it into the inflate dispatch path - Corrected windowBits handling to map wrapper type/history properly - Replaced raw magic-number crc_flag check with IGZIP_DEFLATE constant - Fixed DEBUG-only issues and cleanup edge cases - Debugging Signed-off-by: Olasoji <olasoji.denloye@intel.com>
Signed-off-by: Olasoji <olasoji.denloye@intel.com>
returned eos isnt erronously set. Signed-off-by: Olasoji <olasoji.denloye@intel.com>
Signed-off-by: Olasoji <olasoji.denloye@intel.com>
Signed-off-by: Olasoji <olasoji.denloye@intel.com>
# Conflicts: # zlib_accel.cpp
Signed-off-by: Olasoji <olasoji.denloye@intel.com>
* igzip: fix raw deflate over-consumption via read_in_length
ISAL's stateful inflate pre-loads input in 8-byte word chunks into a
64-bit shift register (read_in). After reaching ISAL_BLOCK_FINISH for
raw deflate (window_bits < 0), read_in_length still holds the bit count
of bytes fetched beyond the true stream end. Shifting right by 3 gives
the exact over-consumed byte count for every avail_in scenario:
avail_in in [1,7] : previous avail_in<8 heuristic approximated this
avail_in == 0 : previous heuristic was blind; now handled
avail_in == 8 : gap in both old heuristics; now handled
avail_in > 8 : multi-frame continuation; now handled
The corrected consumed count is reported back to the caller via
*input_length so that avail_in / next_in on the zlib stream are
advanced accurately, preventing downstream compressed-size and CRC
mismatches (reproduces as ZipException in Java ZipInputStream).
Boundary guard updated: skip the raw-boundary fallback when
*tofixed==1 because the correction has already accounted for any
remaining bytes, so non-zero remaining is legitimate trailing data
rather than un-corrected over-consumption.
BLOCK_INPUT_DONE path (output-buffer-limited mid-stream) retains the
existing Z_DATA_ERROR->zlib-fallback behaviour unchanged.
Previously attempted stateless-probe approach removed entirely; the
read_in_length field is simpler, exact, and has no multi-frame failure
mode.
Validated: 8/8 IGZIPInflateRegressionTest pass; full make run passes
with only the pre-existing ordering-sensitive case 37961; JAR repro
(250 entries, scan + build-init) passes clean.
* rename trailer_overconsumption_fixed -> read_in_correction_applied; remove stale post-reset pre-set
The old name 'trailer_overconsumption_fixed' was a holdover from the
avail_in < 8 heuristic era. Since EXP-6f the fix is driven entirely by
isal_inflate.read_in_length >> 3; the flag only signals that a correction
was applied *within the current inflate call*.
Rename the field, its parameter aliases, and all local variables to
read_in_correction_applied to match the actual semantics.
Also remove the stale block in inflateReset:
if (was_igzip_path) {
inflate_settings->trailer_overconsumption_fixed = 1;
}
This block predated the read_in_length fix. It pre-armed the flag so
that the old avail_in < 8 boundary guard would not fire on the first
call after a reset. That logic was correct under the old heuristic but
is actively wrong now: ResetUncompressIGZIP already zeros the flag;
re-arming it to 1 would suppress the boundary guard on exactly the call
where it is needed most (first call after reset on a raw stream).
Remove was_igzip_path (now unused after the block removal) and inline
the INPUT_DONE log expression to eliminate an unused-variable warning
when DEBUG_LOG=OFF.
No behaviour change for correct streams; test suite: 8/8 IGZIP inflate
regressions PASS, only pre-existing 37961 flaky failure remains.
* add IGZIP inflate/deflate counters to statistics; enable ENABLE_STATISTICS build flag
Statistics previously tracked QAT and IAA paths but left IGZIP with
commented-out placeholders. Two problems with those placeholders: the
enum values DEFLATE_IGZIP_COUNT, DEFLATE_IGZIP_ERROR_COUNT,
INFLATE_IGZIP_COUNT, INFLATE_IGZIP_ERROR_COUNT did not exist, and the
stat_names array had no entries for them.
Add the four missing Statistic enum values (after the IAA entries,
before ZLIB, matching the QAT/IAA grouping pattern) and their
corresponding stat_names strings. Uncomment the INCREMENT_STAT calls
in the deflate and inflate IGZIP dispatch blocks. The error condition
mirrors IAA/QAT: unconditional IGZIP_COUNT on every dispatch, and
IGZIP_ERROR_COUNT when ret != 0 (fallback or hard error).
Enable ENABLE_STATISTICS=ON in cmake.txt so the counters compile in by
default.
Validation (OpenSearch opensearch-core-3.3.1.jar, 250-entry JAR repro,
use_igzip_uncompress=1, log_stats_samples=50):
Thread (main): inflate_count=750 igzip=749 igzip_errors=0 zlib=0
— 99.9% of inflate calls served by IGZIP with zero errors or
zlib fallbacks; one call is the initial probe before path selection.
* add iaa_fallback_igzip: IAA can fall back to IGZIP before software zlib
When IAA inflate or deflate fails (ret != 0), and use_igzip_uncompress /
use_igzip_compress is enabled, the new iaa_fallback_igzip config flag
(default 0) routes the retry to IGZIP before allowing the call to
fall through to software zlib.
Motivation: IAA is stateless and leaves input unconsumed on failure, so
IGZIP can retry from the same position at no extra cost. This keeps
hardware-accelerated decompression in play for inputs that IAA rejects
but IGZIP can handle (e.g. streams that fail IsIAADecompressible or
exceed IAA's single-call constraint).
Implementation:
- New ConfigOption IAA_FALLBACK_IGZIP added between IGNORE_ZLIB_DICTIONARY
and LOG_LEVEL; default 0 (opt-in), range [0,1].
- inflate(): after IAA dispatch block, if path_selected==IAA && ret!=0 &&
configs[IAA_FALLBACK_IGZIP] && igzip_available, reset input_len/output_len
(IAA may have modified them on failure), clear read_in_correction_applied,
and run IGZIPRunInflateAndSelectPathAction with the same path-action
handling as the normal IGZIP dispatch. Falls through to zlib if IGZIP
also fails.
- deflate(): identical pattern; initialises isal_strm via InitCompressIGZIP
if needed (matching normal IGZIP deflate path).
- Both fallback blocks are wrapped in #ifdef USE_IGZIP and guarded by
igzip_available so they compile away and are unreachable without IGZIP.
- Existing INFLATE/DEFLATE_IGZIP_COUNT stats capture fallback calls.
Test suite: 44756/44757 PASS (37961 pre-existing flaky only).
* cmake: set ENABLE_STATISTICS=OFF by default
* - Reset statistics to off by default and formatting
Signed-off-by: Olasoji <olasoji.denloye@intel.com>
* IGZIP: lift Z_FINISH-only restriction; enable full streaming compress
- SupportedOptionsIGZIPCompress: always returns true (ISA-L natively
supports NO_FLUSH/SYNC_FLUSH/FULL_FLUSH/FINISH)
- SupportedOptionsIGZIPUncompress: always returns true (remove dead
zero-input new-stream guard)
- IGZIPShouldFallbackDeflate + IsIGZIPSyncFlush: removed entirely;
both streaming_flush_with_input and empty_sync_flush_reentry cases
were artificial constraints inherited from one-shot IAA/QAT model
- CompressIGZIP: add ZSTATE_NEW_HDR guard for empty SYNC_FLUSH calls;
ISA-L emits sync bytes unconditionally, guard returns 0 progress so
caller sees Z_BUF_ERROR matching zlib semantics
- deflateSetDictionary: reject mid-stream calls when an accelerator
path is active; underlying zlib stream is unadvanced so
orig_deflateSetDictionary would incorrectly return Z_OK
- inflate active-stream no-input block: remove dead
- tests: remove 6 stale path==ZLIB assertions from 3 regression tests
that assumed IGZIP would fall back on streaming flushes
* fix iaa_fallback_igzip compress: set path_selected=IGZIP after fallback
When IAA compress fails and IGZIP is used as a fallback, path_selected
remained IAA. The return-code block below then used the IAA/QAT one-shot
logic (avail_in==0 -> Z_STREAM_END) instead of IGZIP streaming logic
(IsIGZIPDeflateFinished). ISA-L fills its internal level buffer and
returns after consuming all input but producing only partial output;
avail_in reaches 0 while the stream is not yet at ZSTATE_END. The result
was Z_STREAM_END returned with ~48KB of compressed data still buffered
internally, corrupting every document written during indexing.
Fix: set path_selected = IGZIP immediately after the fallback completes
so that the return-code logic correctly calls IsIGZIPDeflateFinished and
returns Z_OK for partial output, allowing the caller to drain the stream.
* iaa: replace marker-based IsIAADecompressible with 512-byte threshold
When iaa_prepend_empty_block=0 (the default), the old code returned true
for all raw deflate and gzip inputs unconditionally, causing QPL's
overconsumption bug to surface for Java ZipInputStream callers. JAR
loading feeds inflate() with 512-byte chunks where avail_in > actual
compressed size; QPL reports total_in == available_in, causing the
Java-level ZipException that kills OpenSearch at launch.
Fix (ported from exp-8): replace the IAA_PREPEND_EMPTY_BLOCK=0 branch
with a 512-byte threshold guard. ZipInputStream always feeds <=512-byte
chunks; Lucene stored-field reads always provide the exact compressed
size which is typically well above 512 bytes. The few Lucene entries
below the threshold fall back to IGZIP, which is also correct.
Also removes PrependedEmptyBlockPresent() which is no longer called
from the decompression path. The compress-side marker write in
CompressIAA is unaffected (still controlled by iaa_prepend_empty_block).
* README: document USE_IGZIP cmake option and ISA-L dependency
Add USE_IGZIP (ON/OFF) and ISAL_PATH to the cmake options list, and
add a Requirements for IGZIP section with a link to ISA-L.
Signed-off-by: Olasoji <olasoji.denloye@intel.com>
* zlib_accel: guard pre_avail_in declaration with USE_IGZIP
pre_avail_in is only referenced inside #ifdef USE_IGZIP blocks.
Without this guard, builds with USE_IGZIP=OFF trigger an
-Wunused-variable error under -Werror, breaking CI.
Signed-off-by: Olasoji <olasoji.denloye@intel.com>
* Clang format
Signed-off-by: Olasoji <olasoji.denloye@intel.com>
* remove cmake.txt; fix null deref in deflateSetDictionary
cmake.txt contained a local build command with hardcoded absolute paths
(/home/sdp/...). Remove it from the repo and add to .gitignore.
deflateSetDictionary: deflate_stream_settings.Get(strm) can return
nullptr if called without a prior deflateInit. Guard the mid-stream
rejection check to avoid a null dereference in that case, returning
Z_STREAM_ERROR consistent with zlib behaviour.
Addresses review comments on PR #54 (matt-welch, Copilot).
Signed-off-by: Olasoji <olasoji.denloye@intel.com>
* igzip: remove dead SupportedOptions/IGZIPShouldFallback stubs
SupportedOptionsIGZIPCompress, SupportedOptionsIGZIPUncompress, and
IGZIPShouldFallbackDeflate unconditionally returned fixed values with
all parameters voided. Remove them entirely and inline the fixed values
at all 8 call sites in zlib_accel.cpp.
Addresses review comment on PR #54 (matt-welch).
Signed-off-by: Olasoji <olasoji.denloye@intel.com>
* zlib_accel: fix read_in_correction_applied semantics and reset asymmetry
The flag is per-stream-session (set once when the read_in_length
over-consumption correction fires; cleared only on inflateReset), not
per-call as the comment incorrectly stated.
Fix the struct comment to reflect this, and remove the erroneous reset
to 0 on the IAA->IGZIP fallback path which was inconsistent with the
primary IGZIP path and the inflateReset behavior.
Addresses review comment on PR #54 (Copilot, matt-welch).
Signed-off-by: Olasoji <olasoji.denloye@intel.com>
* iaa: deprecate iaa_prepend_empty_block config option
The empty stored-block marker approach was abandoned in favour of a
512-byte minimum input length threshold in IsIAADecompressible. The
config option is retained for backward compatibility but has no effect
on decompression and will be removed in a future release.
Rationale for the threshold approach: QPL hardware always consumes all
available_in bytes regardless of where the BFINAL=1 token falls
(overconsumption bug). Java ZipInputStream feeds <=512-byte chunks
when csize is unknown, triggering overconsumption errors. Lucene
stored-field reads always supply the exact compressed size (>512 bytes),
where consuming all input is correct.
Document the deprecation in README and add a comment in iaa.cpp
explaining the history and the replacement mechanism.
Addresses review comment on PR #54 (matt-welch).
Signed-off-by: $(git config user.name) <$(git config user.email)>
* tests: add IAA->IGZIP fallback coverage
Add IAAFallbackIGZIPTest with three tests exercising the fallback
path for both deflate and inflate:
- DeflateUsesIGZIPWhenIAAFailsAndFallbackEnabled: configures
USE_IAA_COMPRESS+USE_IGZIP_COMPRESS+IAA_FALLBACK_IGZIP=1 and
asserts the stream lands on IGZIP (or IAA if hardware present).
- DeflateDoesNotUseIGZIPWhenFallbackDisabled: same IAA+IGZIP config
but IAA_FALLBACK_IGZIP=0; asserts IGZIP is never selected.
- InflateUsesIGZIPWhenIAAFailsAndFallbackEnabled: data compressed
with IGZIP, decompressed via USE_IAA_UNCOMPRESS+IAA_FALLBACK_IGZIP=1;
asserts round-trip correctness and IGZIP (or IAA) execution path.
On machines without IAA hardware the QPL init fails (non-zero return),
which naturally exercises the fallback branch without mocking. On
machines that do have IAA hardware and succeed, the assertions accept
either IAA or IGZIP so the tests remain valid in both environments.
Tests are guarded by #ifdef USE_IAA and #ifdef USE_IGZIP (both are
in scope here via the outer USE_IGZIP guard around the regression
block).
Addresses should-fix #6 from PR #54 review (matt-welch).
Signed-off-by: Olasoji <olasoji.denloye@intel.com>
* tests: restore IGZIP path assertions for SYNC_FLUSH regression tests
Three IGZIPDeflateRegressionTest tests originally asserted
ASSERT_EQ(path, ZLIB) because IGZIPShouldFallbackDeflate redirected
SYNC_FLUSH calls to the zlib path. Commit f7ee0ec (lift Z_FINISH-only
restriction) removed those assertions when IGZIP gained native
SYNC_FLUSH support, but left no replacement.
Now that IGZIPShouldFallbackDeflate is removed entirely, restore the
path assertions with the new expected behavior: IGZIP must remain on
the IGZIP path through Z_NO_FLUSH, Z_SYNC_FLUSH, and Z_FINISH calls.
Changes:
- ResetMustNotStallSyncFlushOnSameStream: assert IGZIP after
Z_NO_FLUSH and Z_SYNC_FLUSH on each cycle.
- RepeatedEmptySyncFlushMustEventuallyReportNoProgress: assert IGZIP
after Z_NO_FLUSH and on every Z_SYNC_FLUSH iteration.
- SyncFlushWithInputMustFallbackToZlibForStreamSafety: renamed to
SyncFlushWithInputMustStayOnIGZIPPath (the old name described the
removed fallback behavior); assert IGZIP after Z_SYNC_FLUSH and on
every Z_FINISH iteration.
Addresses review comment #7 on PR #54 (matt-welch).
Signed-off-by: Olasoji <olasoji.denloye@intel.com>
* igzip: document read_in_correction_applied reset and ZSTATE_NEW_HDR coupling
#8 — Explain why read_in_correction_applied is cleared on inflateReset:
isal_inflate_reset clears the internal read_in/read_in_length buffer,
so any over-consumption correction from the previous stream session no
longer applies. The flag must be reset so the new session can fire the
correction fresh if needed. This is intentional; keeping it set across
a reset would incorrectly suppress the correction on data that has not
yet over-consumed.
#9 — Document ISA-L version for ZSTATE_NEW_HDR coupling:
The SYNC_FLUSH no-progress guard directly accesses
isal_strm->internal_state.state == ZSTATE_NEW_HDR. Note that this was
validated against ISA-L v2.32.0 (commit c196241) so future ISA-L
updates can be audited for state machine changes.
Addresses review comments #8 and #9 on PR #54 (matt-welch).
Signed-off-by: Olasoji <olasoji.denloye@intel.com>
* address round-2 Copilot review: README doc, igzip.h comment, IGZIP stats tests, inflate fallback=0 test
- README.md: document iaa_fallback_igzip config option
- igzip.h: fix read_in_correction_applied comment to reflect per-stream-session
semantics (not per-call)
- tests/zlib_accel_test.cpp: add DEFLATE_IGZIP_COUNT/INFLATE_IGZIP_COUNT
branches to CompressDecompress stats verification
- tests/zlib_accel_test.cpp: add InflateDoesNotUseIGZIPWhenFallbackDisabled
as companion to the existing inflate=1 test
* fix: restore gzip_flag after deflateReset on IGZIP stream (Cassandra corruption)
isal_deflate_reset preserves gzip_flag. After the first chunk, ISA-L
internally changes gzip_flag from IGZIP_ZLIB (3) to IGZIP_ZLIB_NO_HDR (4)
to suppress the header on continuation calls. Without a reset, the reused
stream produced headerless chunks, causing Java's Inflater (nowrap=false)
to report 'incorrect header check' / 'unknown compression method' on every
subsequent SSTable chunk — resulting in CorruptSSTableException at read time.
Fix: add ResetCompressIGZIP() which wraps isal_deflate_reset + calls
ConfigureDeflateWindow to restore gzip_flag and hist_bits from window_bits.
Replace the inline isal_deflate_reset call in deflateReset() with this.
Regression test: ResetMustRestoreZlibHeaderForSubsequentChunks compresses
5 independent chunks via a reused IGZIP z_stream (deflateReset between each)
and decompresses each with a fresh zlib inflater — would have failed before
this fix on chunk 2 onward.
Reproduced via: Cassandra 5 mix workload (80% reads, 20% updates) with
IGZIP compress enabled, writing to a zlib-format SSTable.
---------
Signed-off-by: Olasoji <olasoji.denloye@intel.com>
Signed-off-by: $(git config user.name) <$(git config user.email)>
…ault=1 (#57) * igzip: fix raw deflate over-consumption via read_in_length ISAL's stateful inflate pre-loads input in 8-byte word chunks into a 64-bit shift register (read_in). After reaching ISAL_BLOCK_FINISH for raw deflate (window_bits < 0), read_in_length still holds the bit count of bytes fetched beyond the true stream end. Shifting right by 3 gives the exact over-consumed byte count for every avail_in scenario: avail_in in [1,7] : previous avail_in<8 heuristic approximated this avail_in == 0 : previous heuristic was blind; now handled avail_in == 8 : gap in both old heuristics; now handled avail_in > 8 : multi-frame continuation; now handled The corrected consumed count is reported back to the caller via *input_length so that avail_in / next_in on the zlib stream are advanced accurately, preventing downstream compressed-size and CRC mismatches (reproduces as ZipException in Java ZipInputStream). Boundary guard updated: skip the raw-boundary fallback when *tofixed==1 because the correction has already accounted for any remaining bytes, so non-zero remaining is legitimate trailing data rather than un-corrected over-consumption. BLOCK_INPUT_DONE path (output-buffer-limited mid-stream) retains the existing Z_DATA_ERROR->zlib-fallback behaviour unchanged. Previously attempted stateless-probe approach removed entirely; the read_in_length field is simpler, exact, and has no multi-frame failure mode. Validated: 8/8 IGZIPInflateRegressionTest pass; full make run passes with only the pre-existing ordering-sensitive case 37961; JAR repro (250 entries, scan + build-init) passes clean. * rename trailer_overconsumption_fixed -> read_in_correction_applied; remove stale post-reset pre-set The old name 'trailer_overconsumption_fixed' was a holdover from the avail_in < 8 heuristic era. Since EXP-6f the fix is driven entirely by isal_inflate.read_in_length >> 3; the flag only signals that a correction was applied *within the current inflate call*. Rename the field, its parameter aliases, and all local variables to read_in_correction_applied to match the actual semantics. Also remove the stale block in inflateReset: if (was_igzip_path) { inflate_settings->trailer_overconsumption_fixed = 1; } This block predated the read_in_length fix. It pre-armed the flag so that the old avail_in < 8 boundary guard would not fire on the first call after a reset. That logic was correct under the old heuristic but is actively wrong now: ResetUncompressIGZIP already zeros the flag; re-arming it to 1 would suppress the boundary guard on exactly the call where it is needed most (first call after reset on a raw stream). Remove was_igzip_path (now unused after the block removal) and inline the INPUT_DONE log expression to eliminate an unused-variable warning when DEBUG_LOG=OFF. No behaviour change for correct streams; test suite: 8/8 IGZIP inflate regressions PASS, only pre-existing 37961 flaky failure remains. * add IGZIP inflate/deflate counters to statistics; enable ENABLE_STATISTICS build flag Statistics previously tracked QAT and IAA paths but left IGZIP with commented-out placeholders. Two problems with those placeholders: the enum values DEFLATE_IGZIP_COUNT, DEFLATE_IGZIP_ERROR_COUNT, INFLATE_IGZIP_COUNT, INFLATE_IGZIP_ERROR_COUNT did not exist, and the stat_names array had no entries for them. Add the four missing Statistic enum values (after the IAA entries, before ZLIB, matching the QAT/IAA grouping pattern) and their corresponding stat_names strings. Uncomment the INCREMENT_STAT calls in the deflate and inflate IGZIP dispatch blocks. The error condition mirrors IAA/QAT: unconditional IGZIP_COUNT on every dispatch, and IGZIP_ERROR_COUNT when ret != 0 (fallback or hard error). Enable ENABLE_STATISTICS=ON in cmake.txt so the counters compile in by default. Validation (OpenSearch opensearch-core-3.3.1.jar, 250-entry JAR repro, use_igzip_uncompress=1, log_stats_samples=50): Thread (main): inflate_count=750 igzip=749 igzip_errors=0 zlib=0 — 99.9% of inflate calls served by IGZIP with zero errors or zlib fallbacks; one call is the initial probe before path selection. * add iaa_fallback_igzip: IAA can fall back to IGZIP before software zlib When IAA inflate or deflate fails (ret != 0), and use_igzip_uncompress / use_igzip_compress is enabled, the new iaa_fallback_igzip config flag (default 0) routes the retry to IGZIP before allowing the call to fall through to software zlib. Motivation: IAA is stateless and leaves input unconsumed on failure, so IGZIP can retry from the same position at no extra cost. This keeps hardware-accelerated decompression in play for inputs that IAA rejects but IGZIP can handle (e.g. streams that fail IsIAADecompressible or exceed IAA's single-call constraint). Implementation: - New ConfigOption IAA_FALLBACK_IGZIP added between IGNORE_ZLIB_DICTIONARY and LOG_LEVEL; default 0 (opt-in), range [0,1]. - inflate(): after IAA dispatch block, if path_selected==IAA && ret!=0 && configs[IAA_FALLBACK_IGZIP] && igzip_available, reset input_len/output_len (IAA may have modified them on failure), clear read_in_correction_applied, and run IGZIPRunInflateAndSelectPathAction with the same path-action handling as the normal IGZIP dispatch. Falls through to zlib if IGZIP also fails. - deflate(): identical pattern; initialises isal_strm via InitCompressIGZIP if needed (matching normal IGZIP deflate path). - Both fallback blocks are wrapped in #ifdef USE_IGZIP and guarded by igzip_available so they compile away and are unreachable without IGZIP. - Existing INFLATE/DEFLATE_IGZIP_COUNT stats capture fallback calls. Test suite: 44756/44757 PASS (37961 pre-existing flaky only). * cmake: set ENABLE_STATISTICS=OFF by default * - Reset statistics to off by default and formatting Signed-off-by: Olasoji <olasoji.denloye@intel.com> * IGZIP: lift Z_FINISH-only restriction; enable full streaming compress - SupportedOptionsIGZIPCompress: always returns true (ISA-L natively supports NO_FLUSH/SYNC_FLUSH/FULL_FLUSH/FINISH) - SupportedOptionsIGZIPUncompress: always returns true (remove dead zero-input new-stream guard) - IGZIPShouldFallbackDeflate + IsIGZIPSyncFlush: removed entirely; both streaming_flush_with_input and empty_sync_flush_reentry cases were artificial constraints inherited from one-shot IAA/QAT model - CompressIGZIP: add ZSTATE_NEW_HDR guard for empty SYNC_FLUSH calls; ISA-L emits sync bytes unconditionally, guard returns 0 progress so caller sees Z_BUF_ERROR matching zlib semantics - deflateSetDictionary: reject mid-stream calls when an accelerator path is active; underlying zlib stream is unadvanced so orig_deflateSetDictionary would incorrectly return Z_OK - inflate active-stream no-input block: remove dead - tests: remove 6 stale path==ZLIB assertions from 3 regression tests that assumed IGZIP would fall back on streaming flushes * fix iaa_fallback_igzip compress: set path_selected=IGZIP after fallback When IAA compress fails and IGZIP is used as a fallback, path_selected remained IAA. The return-code block below then used the IAA/QAT one-shot logic (avail_in==0 -> Z_STREAM_END) instead of IGZIP streaming logic (IsIGZIPDeflateFinished). ISA-L fills its internal level buffer and returns after consuming all input but producing only partial output; avail_in reaches 0 while the stream is not yet at ZSTATE_END. The result was Z_STREAM_END returned with ~48KB of compressed data still buffered internally, corrupting every document written during indexing. Fix: set path_selected = IGZIP immediately after the fallback completes so that the return-code logic correctly calls IsIGZIPDeflateFinished and returns Z_OK for partial output, allowing the caller to drain the stream. * iaa: replace marker-based IsIAADecompressible with 512-byte threshold When iaa_prepend_empty_block=0 (the default), the old code returned true for all raw deflate and gzip inputs unconditionally, causing QPL's overconsumption bug to surface for Java ZipInputStream callers. JAR loading feeds inflate() with 512-byte chunks where avail_in > actual compressed size; QPL reports total_in == available_in, causing the Java-level ZipException that kills OpenSearch at launch. Fix (ported from exp-8): replace the IAA_PREPEND_EMPTY_BLOCK=0 branch with a 512-byte threshold guard. ZipInputStream always feeds <=512-byte chunks; Lucene stored-field reads always provide the exact compressed size which is typically well above 512 bytes. The few Lucene entries below the threshold fall back to IGZIP, which is also correct. Also removes PrependedEmptyBlockPresent() which is no longer called from the decompression path. The compress-side marker write in CompressIAA is unaffected (still controlled by iaa_prepend_empty_block). * README: document USE_IGZIP cmake option and ISA-L dependency Add USE_IGZIP (ON/OFF) and ISAL_PATH to the cmake options list, and add a Requirements for IGZIP section with a link to ISA-L. Signed-off-by: Olasoji <olasoji.denloye@intel.com> * zlib_accel: guard pre_avail_in declaration with USE_IGZIP pre_avail_in is only referenced inside #ifdef USE_IGZIP blocks. Without this guard, builds with USE_IGZIP=OFF trigger an -Wunused-variable error under -Werror, breaking CI. Signed-off-by: Olasoji <olasoji.denloye@intel.com> * Clang format Signed-off-by: Olasoji <olasoji.denloye@intel.com> * remove cmake.txt; fix null deref in deflateSetDictionary cmake.txt contained a local build command with hardcoded absolute paths (/home/sdp/...). Remove it from the repo and add to .gitignore. deflateSetDictionary: deflate_stream_settings.Get(strm) can return nullptr if called without a prior deflateInit. Guard the mid-stream rejection check to avoid a null dereference in that case, returning Z_STREAM_ERROR consistent with zlib behaviour. Addresses review comments on PR #54 (matt-welch, Copilot). Signed-off-by: Olasoji <olasoji.denloye@intel.com> * igzip: remove dead SupportedOptions/IGZIPShouldFallback stubs SupportedOptionsIGZIPCompress, SupportedOptionsIGZIPUncompress, and IGZIPShouldFallbackDeflate unconditionally returned fixed values with all parameters voided. Remove them entirely and inline the fixed values at all 8 call sites in zlib_accel.cpp. Addresses review comment on PR #54 (matt-welch). Signed-off-by: Olasoji <olasoji.denloye@intel.com> * zlib_accel: fix read_in_correction_applied semantics and reset asymmetry The flag is per-stream-session (set once when the read_in_length over-consumption correction fires; cleared only on inflateReset), not per-call as the comment incorrectly stated. Fix the struct comment to reflect this, and remove the erroneous reset to 0 on the IAA->IGZIP fallback path which was inconsistent with the primary IGZIP path and the inflateReset behavior. Addresses review comment on PR #54 (Copilot, matt-welch). Signed-off-by: Olasoji <olasoji.denloye@intel.com> * iaa: deprecate iaa_prepend_empty_block config option The empty stored-block marker approach was abandoned in favour of a 512-byte minimum input length threshold in IsIAADecompressible. The config option is retained for backward compatibility but has no effect on decompression and will be removed in a future release. Rationale for the threshold approach: QPL hardware always consumes all available_in bytes regardless of where the BFINAL=1 token falls (overconsumption bug). Java ZipInputStream feeds <=512-byte chunks when csize is unknown, triggering overconsumption errors. Lucene stored-field reads always supply the exact compressed size (>512 bytes), where consuming all input is correct. Document the deprecation in README and add a comment in iaa.cpp explaining the history and the replacement mechanism. Addresses review comment on PR #54 (matt-welch). Signed-off-by: $(git config user.name) <$(git config user.email)> * tests: add IAA->IGZIP fallback coverage Add IAAFallbackIGZIPTest with three tests exercising the fallback path for both deflate and inflate: - DeflateUsesIGZIPWhenIAAFailsAndFallbackEnabled: configures USE_IAA_COMPRESS+USE_IGZIP_COMPRESS+IAA_FALLBACK_IGZIP=1 and asserts the stream lands on IGZIP (or IAA if hardware present). - DeflateDoesNotUseIGZIPWhenFallbackDisabled: same IAA+IGZIP config but IAA_FALLBACK_IGZIP=0; asserts IGZIP is never selected. - InflateUsesIGZIPWhenIAAFailsAndFallbackEnabled: data compressed with IGZIP, decompressed via USE_IAA_UNCOMPRESS+IAA_FALLBACK_IGZIP=1; asserts round-trip correctness and IGZIP (or IAA) execution path. On machines without IAA hardware the QPL init fails (non-zero return), which naturally exercises the fallback branch without mocking. On machines that do have IAA hardware and succeed, the assertions accept either IAA or IGZIP so the tests remain valid in both environments. Tests are guarded by #ifdef USE_IAA and #ifdef USE_IGZIP (both are in scope here via the outer USE_IGZIP guard around the regression block). Addresses should-fix #6 from PR #54 review (matt-welch). Signed-off-by: Olasoji <olasoji.denloye@intel.com> * tests: restore IGZIP path assertions for SYNC_FLUSH regression tests Three IGZIPDeflateRegressionTest tests originally asserted ASSERT_EQ(path, ZLIB) because IGZIPShouldFallbackDeflate redirected SYNC_FLUSH calls to the zlib path. Commit f7ee0ec (lift Z_FINISH-only restriction) removed those assertions when IGZIP gained native SYNC_FLUSH support, but left no replacement. Now that IGZIPShouldFallbackDeflate is removed entirely, restore the path assertions with the new expected behavior: IGZIP must remain on the IGZIP path through Z_NO_FLUSH, Z_SYNC_FLUSH, and Z_FINISH calls. Changes: - ResetMustNotStallSyncFlushOnSameStream: assert IGZIP after Z_NO_FLUSH and Z_SYNC_FLUSH on each cycle. - RepeatedEmptySyncFlushMustEventuallyReportNoProgress: assert IGZIP after Z_NO_FLUSH and on every Z_SYNC_FLUSH iteration. - SyncFlushWithInputMustFallbackToZlibForStreamSafety: renamed to SyncFlushWithInputMustStayOnIGZIPPath (the old name described the removed fallback behavior); assert IGZIP after Z_SYNC_FLUSH and on every Z_FINISH iteration. Addresses review comment #7 on PR #54 (matt-welch). Signed-off-by: Olasoji <olasoji.denloye@intel.com> * igzip: document read_in_correction_applied reset and ZSTATE_NEW_HDR coupling #8 — Explain why read_in_correction_applied is cleared on inflateReset: isal_inflate_reset clears the internal read_in/read_in_length buffer, so any over-consumption correction from the previous stream session no longer applies. The flag must be reset so the new session can fire the correction fresh if needed. This is intentional; keeping it set across a reset would incorrectly suppress the correction on data that has not yet over-consumed. #9 — Document ISA-L version for ZSTATE_NEW_HDR coupling: The SYNC_FLUSH no-progress guard directly accesses isal_strm->internal_state.state == ZSTATE_NEW_HDR. Note that this was validated against ISA-L v2.32.0 (commit c196241) so future ISA-L updates can be audited for state machine changes. Addresses review comments #8 and #9 on PR #54 (matt-welch). Signed-off-by: Olasoji <olasoji.denloye@intel.com> * address round-2 Copilot review: README doc, igzip.h comment, IGZIP stats tests, inflate fallback=0 test - README.md: document iaa_fallback_igzip config option - igzip.h: fix read_in_correction_applied comment to reflect per-stream-session semantics (not per-call) - tests/zlib_accel_test.cpp: add DEFLATE_IGZIP_COUNT/INFLATE_IGZIP_COUNT branches to CompressDecompress stats verification - tests/zlib_accel_test.cpp: add InflateDoesNotUseIGZIPWhenFallbackDisabled as companion to the existing inflate=1 test * fix: restore gzip_flag after deflateReset on IGZIP stream (Cassandra corruption) isal_deflate_reset preserves gzip_flag. After the first chunk, ISA-L internally changes gzip_flag from IGZIP_ZLIB (3) to IGZIP_ZLIB_NO_HDR (4) to suppress the header on continuation calls. Without a reset, the reused stream produced headerless chunks, causing Java's Inflater (nowrap=false) to report 'incorrect header check' / 'unknown compression method' on every subsequent SSTable chunk — resulting in CorruptSSTableException at read time. Fix: add ResetCompressIGZIP() which wraps isal_deflate_reset + calls ConfigureDeflateWindow to restore gzip_flag and hist_bits from window_bits. Replace the inline isal_deflate_reset call in deflateReset() with this. Regression test: ResetMustRestoreZlibHeaderForSubsequentChunks compresses 5 independent chunks via a reused IGZIP z_stream (deflateReset between each) and decompresses each with a fresh zlib inflater — would have failed before this fix on chunk 2 onward. Reproduced via: Cassandra 5 mix workload (80% reads, 20% updates) with IGZIP compress enabled, writing to a zlib-format SSTable. * Rename iaa_fallback_igzip to igzip_fallback, extend to QAT, default=1 - Rename IAA_FALLBACK_IGZIP -> IGZIP_FALLBACK (single generic key) - Extend fallback condition in deflate() and inflate() to cover QAT: (path_selected == IAA || path_selected == QAT) - Move failed_accelerator declaration before IGZIP retry call in both blocks - Log messages updated to identify the failing accelerator (IAA or QAT) - Flip default from 0 -> 1 (fallback is now on by default when IGZIP enabled) - Update README: key name, description, default - Add QATFallbackIGZIPTest suite (4 tests mirroring IAAFallbackIGZIPTest) Full make run: 51006 tests, 44766 passed, remainder expected skips. * fix: build error when USE_IGZIP + USE_IAA/USE_QAT all enabled SetDeflatePath/SetInflatePath take const char*, but commit 8120b29 passed std::string temporaries (string concatenation) which is a hard compile error. The reason parameter is (void)'d in both functions, so replace the broken concatenations with plain string literals and remove the now-unused failed_accelerator variables. The error was masked because the affected code is inside #ifdef USE_IGZIP blocks that are only reached when both an accelerator (IAA or QAT) and IGZIP are compiled in together. Builds with fewer accelerators enabled did not trigger it. * tests: assert return codes and guard QAT->IGZIP tests with USE_IGZIP - Add ASSERT_EQ(ret, Z_STREAM_END) for deflate/inflate calls in QATFallbackIGZIPTest::DeflateDoesNotUseIGZIPWhenFallbackDisabled and InflateDoesNotUseIGZIPWhenFallbackDisabled so unchecked return codes are caught as test failures. - Guard QATFallbackIGZIPTest block with #if defined(USE_QAT) && defined(USE_IGZIP) since these tests exercise the QAT->IGZIP fallback path which requires both backends to be compiled in; USE_QAT alone is insufficient. * fixup: use const char* ternaries to identify accelerator in fallback reasons Address third-round Copilot review comments on PR #57: - SetDeflatePath: use path_selected ternary to say "IAA failed" vs "QAT failed" instead of the generic "accelerator failed" literal. - SetInflatePath (accelerator->IGZIP fallback block): declare failed_accelerator const char* from path_selected, then use ternaries in all four path-action branches so log and reason strings name the failing accelerator (IAA/QAT). - IAAFallbackIGZIPTest.DeflateDoesNotUseIGZIPWhenFallbackDisabled: capture and assert Z_STREAM_END from deflate() so compression failures are caught. All 44766 tests pass. --------- Signed-off-by: Olasoji <olasoji.denloye@intel.com> Signed-off-by: $(git config user.name) <$(git config user.email)>
* igzip: fix raw deflate over-consumption via read_in_length
ISAL's stateful inflate pre-loads input in 8-byte word chunks into a
64-bit shift register (read_in). After reaching ISAL_BLOCK_FINISH for
raw deflate (window_bits < 0), read_in_length still holds the bit count
of bytes fetched beyond the true stream end. Shifting right by 3 gives
the exact over-consumed byte count for every avail_in scenario:
avail_in in [1,7] : previous avail_in<8 heuristic approximated this
avail_in == 0 : previous heuristic was blind; now handled
avail_in == 8 : gap in both old heuristics; now handled
avail_in > 8 : multi-frame continuation; now handled
The corrected consumed count is reported back to the caller via
*input_length so that avail_in / next_in on the zlib stream are
advanced accurately, preventing downstream compressed-size and CRC
mismatches (reproduces as ZipException in Java ZipInputStream).
Boundary guard updated: skip the raw-boundary fallback when
*tofixed==1 because the correction has already accounted for any
remaining bytes, so non-zero remaining is legitimate trailing data
rather than un-corrected over-consumption.
BLOCK_INPUT_DONE path (output-buffer-limited mid-stream) retains the
existing Z_DATA_ERROR->zlib-fallback behaviour unchanged.
Previously attempted stateless-probe approach removed entirely; the
read_in_length field is simpler, exact, and has no multi-frame failure
mode.
Validated: 8/8 IGZIPInflateRegressionTest pass; full make run passes
with only the pre-existing ordering-sensitive case 37961; JAR repro
(250 entries, scan + build-init) passes clean.
* rename trailer_overconsumption_fixed -> read_in_correction_applied; remove stale post-reset pre-set
The old name 'trailer_overconsumption_fixed' was a holdover from the
avail_in < 8 heuristic era. Since EXP-6f the fix is driven entirely by
isal_inflate.read_in_length >> 3; the flag only signals that a correction
was applied *within the current inflate call*.
Rename the field, its parameter aliases, and all local variables to
read_in_correction_applied to match the actual semantics.
Also remove the stale block in inflateReset:
if (was_igzip_path) {
inflate_settings->trailer_overconsumption_fixed = 1;
}
This block predated the read_in_length fix. It pre-armed the flag so
that the old avail_in < 8 boundary guard would not fire on the first
call after a reset. That logic was correct under the old heuristic but
is actively wrong now: ResetUncompressIGZIP already zeros the flag;
re-arming it to 1 would suppress the boundary guard on exactly the call
where it is needed most (first call after reset on a raw stream).
Remove was_igzip_path (now unused after the block removal) and inline
the INPUT_DONE log expression to eliminate an unused-variable warning
when DEBUG_LOG=OFF.
No behaviour change for correct streams; test suite: 8/8 IGZIP inflate
regressions PASS, only pre-existing 37961 flaky failure remains.
* add IGZIP inflate/deflate counters to statistics; enable ENABLE_STATISTICS build flag
Statistics previously tracked QAT and IAA paths but left IGZIP with
commented-out placeholders. Two problems with those placeholders: the
enum values DEFLATE_IGZIP_COUNT, DEFLATE_IGZIP_ERROR_COUNT,
INFLATE_IGZIP_COUNT, INFLATE_IGZIP_ERROR_COUNT did not exist, and the
stat_names array had no entries for them.
Add the four missing Statistic enum values (after the IAA entries,
before ZLIB, matching the QAT/IAA grouping pattern) and their
corresponding stat_names strings. Uncomment the INCREMENT_STAT calls
in the deflate and inflate IGZIP dispatch blocks. The error condition
mirrors IAA/QAT: unconditional IGZIP_COUNT on every dispatch, and
IGZIP_ERROR_COUNT when ret != 0 (fallback or hard error).
Enable ENABLE_STATISTICS=ON in cmake.txt so the counters compile in by
default.
Validation (OpenSearch opensearch-core-3.3.1.jar, 250-entry JAR repro,
use_igzip_uncompress=1, log_stats_samples=50):
Thread (main): inflate_count=750 igzip=749 igzip_errors=0 zlib=0
— 99.9% of inflate calls served by IGZIP with zero errors or
zlib fallbacks; one call is the initial probe before path selection.
* add iaa_fallback_igzip: IAA can fall back to IGZIP before software zlib
When IAA inflate or deflate fails (ret != 0), and use_igzip_uncompress /
use_igzip_compress is enabled, the new iaa_fallback_igzip config flag
(default 0) routes the retry to IGZIP before allowing the call to
fall through to software zlib.
Motivation: IAA is stateless and leaves input unconsumed on failure, so
IGZIP can retry from the same position at no extra cost. This keeps
hardware-accelerated decompression in play for inputs that IAA rejects
but IGZIP can handle (e.g. streams that fail IsIAADecompressible or
exceed IAA's single-call constraint).
Implementation:
- New ConfigOption IAA_FALLBACK_IGZIP added between IGNORE_ZLIB_DICTIONARY
and LOG_LEVEL; default 0 (opt-in), range [0,1].
- inflate(): after IAA dispatch block, if path_selected==IAA && ret!=0 &&
configs[IAA_FALLBACK_IGZIP] && igzip_available, reset input_len/output_len
(IAA may have modified them on failure), clear read_in_correction_applied,
and run IGZIPRunInflateAndSelectPathAction with the same path-action
handling as the normal IGZIP dispatch. Falls through to zlib if IGZIP
also fails.
- deflate(): identical pattern; initialises isal_strm via InitCompressIGZIP
if needed (matching normal IGZIP deflate path).
- Both fallback blocks are wrapped in #ifdef USE_IGZIP and guarded by
igzip_available so they compile away and are unreachable without IGZIP.
- Existing INFLATE/DEFLATE_IGZIP_COUNT stats capture fallback calls.
Test suite: 44756/44757 PASS (37961 pre-existing flaky only).
* cmake: set ENABLE_STATISTICS=OFF by default
* - Reset statistics to off by default and formatting
Signed-off-by: Olasoji <olasoji.denloye@intel.com>
* IGZIP: lift Z_FINISH-only restriction; enable full streaming compress
- SupportedOptionsIGZIPCompress: always returns true (ISA-L natively
supports NO_FLUSH/SYNC_FLUSH/FULL_FLUSH/FINISH)
- SupportedOptionsIGZIPUncompress: always returns true (remove dead
zero-input new-stream guard)
- IGZIPShouldFallbackDeflate + IsIGZIPSyncFlush: removed entirely;
both streaming_flush_with_input and empty_sync_flush_reentry cases
were artificial constraints inherited from one-shot IAA/QAT model
- CompressIGZIP: add ZSTATE_NEW_HDR guard for empty SYNC_FLUSH calls;
ISA-L emits sync bytes unconditionally, guard returns 0 progress so
caller sees Z_BUF_ERROR matching zlib semantics
- deflateSetDictionary: reject mid-stream calls when an accelerator
path is active; underlying zlib stream is unadvanced so
orig_deflateSetDictionary would incorrectly return Z_OK
- inflate active-stream no-input block: remove dead
- tests: remove 6 stale path==ZLIB assertions from 3 regression tests
that assumed IGZIP would fall back on streaming flushes
* fix iaa_fallback_igzip compress: set path_selected=IGZIP after fallback
When IAA compress fails and IGZIP is used as a fallback, path_selected
remained IAA. The return-code block below then used the IAA/QAT one-shot
logic (avail_in==0 -> Z_STREAM_END) instead of IGZIP streaming logic
(IsIGZIPDeflateFinished). ISA-L fills its internal level buffer and
returns after consuming all input but producing only partial output;
avail_in reaches 0 while the stream is not yet at ZSTATE_END. The result
was Z_STREAM_END returned with ~48KB of compressed data still buffered
internally, corrupting every document written during indexing.
Fix: set path_selected = IGZIP immediately after the fallback completes
so that the return-code logic correctly calls IsIGZIPDeflateFinished and
returns Z_OK for partial output, allowing the caller to drain the stream.
* iaa: replace marker-based IsIAADecompressible with 512-byte threshold
When iaa_prepend_empty_block=0 (the default), the old code returned true
for all raw deflate and gzip inputs unconditionally, causing QPL's
overconsumption bug to surface for Java ZipInputStream callers. JAR
loading feeds inflate() with 512-byte chunks where avail_in > actual
compressed size; QPL reports total_in == available_in, causing the
Java-level ZipException that kills OpenSearch at launch.
Fix (ported from exp-8): replace the IAA_PREPEND_EMPTY_BLOCK=0 branch
with a 512-byte threshold guard. ZipInputStream always feeds <=512-byte
chunks; Lucene stored-field reads always provide the exact compressed
size which is typically well above 512 bytes. The few Lucene entries
below the threshold fall back to IGZIP, which is also correct.
Also removes PrependedEmptyBlockPresent() which is no longer called
from the decompression path. The compress-side marker write in
CompressIAA is unaffected (still controlled by iaa_prepend_empty_block).
* README: document USE_IGZIP cmake option and ISA-L dependency
Add USE_IGZIP (ON/OFF) and ISAL_PATH to the cmake options list, and
add a Requirements for IGZIP section with a link to ISA-L.
Signed-off-by: Olasoji <olasoji.denloye@intel.com>
* zlib_accel: guard pre_avail_in declaration with USE_IGZIP
pre_avail_in is only referenced inside #ifdef USE_IGZIP blocks.
Without this guard, builds with USE_IGZIP=OFF trigger an
-Wunused-variable error under -Werror, breaking CI.
Signed-off-by: Olasoji <olasoji.denloye@intel.com>
* Clang format
Signed-off-by: Olasoji <olasoji.denloye@intel.com>
* remove cmake.txt; fix null deref in deflateSetDictionary
cmake.txt contained a local build command with hardcoded absolute paths
(/home/sdp/...). Remove it from the repo and add to .gitignore.
deflateSetDictionary: deflate_stream_settings.Get(strm) can return
nullptr if called without a prior deflateInit. Guard the mid-stream
rejection check to avoid a null dereference in that case, returning
Z_STREAM_ERROR consistent with zlib behaviour.
Addresses review comments on PR #54 (matt-welch, Copilot).
Signed-off-by: Olasoji <olasoji.denloye@intel.com>
* igzip: remove dead SupportedOptions/IGZIPShouldFallback stubs
SupportedOptionsIGZIPCompress, SupportedOptionsIGZIPUncompress, and
IGZIPShouldFallbackDeflate unconditionally returned fixed values with
all parameters voided. Remove them entirely and inline the fixed values
at all 8 call sites in zlib_accel.cpp.
Addresses review comment on PR #54 (matt-welch).
Signed-off-by: Olasoji <olasoji.denloye@intel.com>
* zlib_accel: fix read_in_correction_applied semantics and reset asymmetry
The flag is per-stream-session (set once when the read_in_length
over-consumption correction fires; cleared only on inflateReset), not
per-call as the comment incorrectly stated.
Fix the struct comment to reflect this, and remove the erroneous reset
to 0 on the IAA->IGZIP fallback path which was inconsistent with the
primary IGZIP path and the inflateReset behavior.
Addresses review comment on PR #54 (Copilot, matt-welch).
Signed-off-by: Olasoji <olasoji.denloye@intel.com>
* iaa: deprecate iaa_prepend_empty_block config option
The empty stored-block marker approach was abandoned in favour of a
512-byte minimum input length threshold in IsIAADecompressible. The
config option is retained for backward compatibility but has no effect
on decompression and will be removed in a future release.
Rationale for the threshold approach: QPL hardware always consumes all
available_in bytes regardless of where the BFINAL=1 token falls
(overconsumption bug). Java ZipInputStream feeds <=512-byte chunks
when csize is unknown, triggering overconsumption errors. Lucene
stored-field reads always supply the exact compressed size (>512 bytes),
where consuming all input is correct.
Document the deprecation in README and add a comment in iaa.cpp
explaining the history and the replacement mechanism.
Addresses review comment on PR #54 (matt-welch).
Signed-off-by: $(git config user.name) <$(git config user.email)>
* tests: add IAA->IGZIP fallback coverage
Add IAAFallbackIGZIPTest with three tests exercising the fallback
path for both deflate and inflate:
- DeflateUsesIGZIPWhenIAAFailsAndFallbackEnabled: configures
USE_IAA_COMPRESS+USE_IGZIP_COMPRESS+IAA_FALLBACK_IGZIP=1 and
asserts the stream lands on IGZIP (or IAA if hardware present).
- DeflateDoesNotUseIGZIPWhenFallbackDisabled: same IAA+IGZIP config
but IAA_FALLBACK_IGZIP=0; asserts IGZIP is never selected.
- InflateUsesIGZIPWhenIAAFailsAndFallbackEnabled: data compressed
with IGZIP, decompressed via USE_IAA_UNCOMPRESS+IAA_FALLBACK_IGZIP=1;
asserts round-trip correctness and IGZIP (or IAA) execution path.
On machines without IAA hardware the QPL init fails (non-zero return),
which naturally exercises the fallback branch without mocking. On
machines that do have IAA hardware and succeed, the assertions accept
either IAA or IGZIP so the tests remain valid in both environments.
Tests are guarded by #ifdef USE_IAA and #ifdef USE_IGZIP (both are
in scope here via the outer USE_IGZIP guard around the regression
block).
Addresses should-fix #6 from PR #54 review (matt-welch).
Signed-off-by: Olasoji <olasoji.denloye@intel.com>
* tests: restore IGZIP path assertions for SYNC_FLUSH regression tests
Three IGZIPDeflateRegressionTest tests originally asserted
ASSERT_EQ(path, ZLIB) because IGZIPShouldFallbackDeflate redirected
SYNC_FLUSH calls to the zlib path. Commit f7ee0ec (lift Z_FINISH-only
restriction) removed those assertions when IGZIP gained native
SYNC_FLUSH support, but left no replacement.
Now that IGZIPShouldFallbackDeflate is removed entirely, restore the
path assertions with the new expected behavior: IGZIP must remain on
the IGZIP path through Z_NO_FLUSH, Z_SYNC_FLUSH, and Z_FINISH calls.
Changes:
- ResetMustNotStallSyncFlushOnSameStream: assert IGZIP after
Z_NO_FLUSH and Z_SYNC_FLUSH on each cycle.
- RepeatedEmptySyncFlushMustEventuallyReportNoProgress: assert IGZIP
after Z_NO_FLUSH and on every Z_SYNC_FLUSH iteration.
- SyncFlushWithInputMustFallbackToZlibForStreamSafety: renamed to
SyncFlushWithInputMustStayOnIGZIPPath (the old name described the
removed fallback behavior); assert IGZIP after Z_SYNC_FLUSH and on
every Z_FINISH iteration.
Addresses review comment #7 on PR #54 (matt-welch).
Signed-off-by: Olasoji <olasoji.denloye@intel.com>
* igzip: document read_in_correction_applied reset and ZSTATE_NEW_HDR coupling
#8 — Explain why read_in_correction_applied is cleared on inflateReset:
isal_inflate_reset clears the internal read_in/read_in_length buffer,
so any over-consumption correction from the previous stream session no
longer applies. The flag must be reset so the new session can fire the
correction fresh if needed. This is intentional; keeping it set across
a reset would incorrectly suppress the correction on data that has not
yet over-consumed.
#9 — Document ISA-L version for ZSTATE_NEW_HDR coupling:
The SYNC_FLUSH no-progress guard directly accesses
isal_strm->internal_state.state == ZSTATE_NEW_HDR. Note that this was
validated against ISA-L v2.32.0 (commit c196241) so future ISA-L
updates can be audited for state machine changes.
Addresses review comments #8 and #9 on PR #54 (matt-welch).
Signed-off-by: Olasoji <olasoji.denloye@intel.com>
* address round-2 Copilot review: README doc, igzip.h comment, IGZIP stats tests, inflate fallback=0 test
- README.md: document iaa_fallback_igzip config option
- igzip.h: fix read_in_correction_applied comment to reflect per-stream-session
semantics (not per-call)
- tests/zlib_accel_test.cpp: add DEFLATE_IGZIP_COUNT/INFLATE_IGZIP_COUNT
branches to CompressDecompress stats verification
- tests/zlib_accel_test.cpp: add InflateDoesNotUseIGZIPWhenFallbackDisabled
as companion to the existing inflate=1 test
* fix: restore gzip_flag after deflateReset on IGZIP stream (Cassandra corruption)
isal_deflate_reset preserves gzip_flag. After the first chunk, ISA-L
internally changes gzip_flag from IGZIP_ZLIB (3) to IGZIP_ZLIB_NO_HDR (4)
to suppress the header on continuation calls. Without a reset, the reused
stream produced headerless chunks, causing Java's Inflater (nowrap=false)
to report 'incorrect header check' / 'unknown compression method' on every
subsequent SSTable chunk — resulting in CorruptSSTableException at read time.
Fix: add ResetCompressIGZIP() which wraps isal_deflate_reset + calls
ConfigureDeflateWindow to restore gzip_flag and hist_bits from window_bits.
Replace the inline isal_deflate_reset call in deflateReset() with this.
Regression test: ResetMustRestoreZlibHeaderForSubsequentChunks compresses
5 independent chunks via a reused IGZIP z_stream (deflateReset between each)
and decompresses each with a fresh zlib inflater — would have failed before
this fix on chunk 2 onward.
Reproduced via: Cassandra 5 mix workload (80% reads, 20% updates) with
IGZIP compress enabled, writing to a zlib-format SSTable.
* Rename iaa_fallback_igzip to igzip_fallback, extend to QAT, default=1
- Rename IAA_FALLBACK_IGZIP -> IGZIP_FALLBACK (single generic key)
- Extend fallback condition in deflate() and inflate() to cover QAT:
(path_selected == IAA || path_selected == QAT)
- Move failed_accelerator declaration before IGZIP retry call in both blocks
- Log messages updated to identify the failing accelerator (IAA or QAT)
- Flip default from 0 -> 1 (fallback is now on by default when IGZIP enabled)
- Update README: key name, description, default
- Add QATFallbackIGZIPTest suite (4 tests mirroring IAAFallbackIGZIPTest)
Full make run: 51006 tests, 44766 passed, remainder expected skips.
* fix: build error when USE_IGZIP + USE_IAA/USE_QAT all enabled
SetDeflatePath/SetInflatePath take const char*, but commit 8120b29
passed std::string temporaries (string concatenation) which is a
hard compile error. The reason parameter is (void)'d in both
functions, so replace the broken concatenations with plain string
literals and remove the now-unused failed_accelerator variables.
The error was masked because the affected code is inside
#ifdef USE_IGZIP blocks that are only reached when both an
accelerator (IAA or QAT) and IGZIP are compiled in together.
Builds with fewer accelerators enabled did not trigger it.
* tests: assert return codes and guard QAT->IGZIP tests with USE_IGZIP
- Add ASSERT_EQ(ret, Z_STREAM_END) for deflate/inflate calls in
QATFallbackIGZIPTest::DeflateDoesNotUseIGZIPWhenFallbackDisabled and
InflateDoesNotUseIGZIPWhenFallbackDisabled so unchecked return codes
are caught as test failures.
- Guard QATFallbackIGZIPTest block with #if defined(USE_QAT) && defined(USE_IGZIP)
since these tests exercise the QAT->IGZIP fallback path which requires
both backends to be compiled in; USE_QAT alone is insufficient.
* fixup: use const char* ternaries to identify accelerator in fallback reasons
Address third-round Copilot review comments on PR #57:
- SetDeflatePath: use path_selected ternary to say "IAA failed" vs "QAT failed"
instead of the generic "accelerator failed" literal.
- SetInflatePath (accelerator->IGZIP fallback block): declare failed_accelerator
const char* from path_selected, then use ternaries in all four path-action
branches so log and reason strings name the failing accelerator (IAA/QAT).
- IAAFallbackIGZIPTest.DeflateDoesNotUseIGZIPWhenFallbackDisabled: capture and
assert Z_STREAM_END from deflate() so compression failures are caught.
All 44766 tests pass.
* igzip: remove workarounds for ISA-L bugs 1–3, 5 (fixed in public v2.32.1)
ISA-L v2.32.1 (released 2026-07-01, tag v2.32.1 on intel/isa-l) contains
official fixes for four bugs that had local workarounds in zlib-accel:
Bug 1 — avail_in over-consumption at ISAL_BLOCK_FINISH (commit 848daed)
ISA-L now rewinds next_in/avail_in by read_in_length/8 bytes after a
block finishes. The read_in_correction_applied workaround and the
sub-byte fallback to software zlib are both removed.
Bug 2 — ISAL_BLOCK_INPUT_DONE guard for concatenated streams (commit 848daed)
A consequence of the Bug 1 fix: BLOCK_FINISH now restores avail_in
correctly, so the pre-emptive fallback at BLOCK_INPUT_DONE is no longer
needed. The guard block is removed.
Bug 3 — gzip_flag mutation after write_stream_header_stateless (commit d8da2d0)
ISA-L no longer mutates gzip_flag from IGZIP_ZLIB→IGZIP_ZLIB_NO_HDR
(or IGZIP_GZIP→IGZIP_GZIP_NO_HDR) after writing the first header.
isal_deflate_reset now preserves a valid gzip_flag, so the explicit
ConfigureDeflateWindow call in ResetCompressIGZIP is removed.
Bug 5 — dict_id little-endian (commit 95b7c35)
ISA-L now stores DICTID in big-endian order matching RFC 1950. The
__builtin_bswap32 workaround in the NEED_DICT handling path is removed.
Validated: 44,766/44,766 tests pass against ISA-L v2.32.1.
* cmake: require ISA-L >= 2.32.1 when USE_IGZIP=ON
ISA-L v2.32.1 (released 2026-07-01) contains critical bugfixes for
igzip stream reuse (gzip_flag mutation, avail_in over-consumption,
dict_id endianness). Earlier versions produce incorrect output with
zlib-accel's streaming deflate/inflate paths.
Adds a CMake configure-time check that parses MAJOR/MINOR/PATCH from
${ISAL_PATH}/include/isal_api.h and fails with a clear message if the
version is older than 2.32.1.
* format: apply clang-format
* address Copilot review comments on PR #60
zlib_accel.cpp: return Z_STREAM_END when end_of_stream is set but no
I/O progress was made (input_len==0 && output_len==0). The previous
code fell through to Z_BUF_ERROR, which could cause callers to miss
the end-of-stream signal on a final trailer-flush step.
common.cmake: harden ISA-L version macro parsing. Add an explicit
FATAL_ERROR when any ISAL_MAJOR/MINOR/PATCH_VERSION macro is missing
from isal_api.h (previously produced a silent '..'-version string).
Use list(GET ... 0) to normalize before regex-replace, guarding
against multiple matches in a malformed header.
* cmake,igzip: address round-3 Copilot review comments on PR #60
- common.cmake: make ISAL_PATH optional — fall back to find_path() for
system-installed ISA-L, consistent with the existing QPL_PATH / USE_IAA
pattern. ISAL_PATH still takes priority when set.
- common.cmake: use whitespace-tolerant regexes ([ \t]+) in both
file(STRINGS) and string(REGEX REPLACE) calls so tabs or multiple
spaces in isal_api.h do not silently produce a malformed version string.
- common.cmake: add per-component numeric validation before composing
_isal_version_found, giving a clear error if parsing fails.
- igzip.cpp: replace /tmp/ ephemeral-file citation in Bug 2 comment with
a reference to the configure-time version guard in common.cmake.
* igzip: address round-4 vkarpenk review comments on PR #60
- CompressIGZIP, UncompressIGZIP: add const to input, total_in,
total_out parameters; all three are only read, never written back.
Use const_cast<uint8_t*> when assigning to ISA-L's non-const
next_in field (C API limitation).
- CompressIGZIP: remove stale (void)total_in/(void)total_out
suppression casts; both parameters are actively read.
- Remove IGZIP_INFLATE_PATH_FALLBACK_RAW_BOUNDARY from enum;
the corresponding code path was already removed from igzip.cpp.
* Formatted code
Signed-off-by: Olasoji <olasoji.denloye@intel.com>
---------
Signed-off-by: Olasoji <olasoji.denloye@intel.com>
Signed-off-by: $(git config user.name) <$(git config user.email)>
* logging: add LOG_DEBUG level; qat: map log level to QATzip log level (#56) * Adds test to validate log_stats_samples config Signed-off-by: Olasoji <olasoji.denloye@intel.com> * Reduces excessive error logging in QAT mode Signed-off-by: Olasoji <olasoji.denloye@intel.com> * Bugfix for the log stream Signed-off-by: Olasoji <olasoji.denloye@intel.com> * Treats QPL_STS_MORE_OUTPUT_NEEDED as a non error condition for IAA Signed-off-by: Olasoji <olasoji.denloye@intel.com> * Fixed formatting Signed-off-by: Olasoji <olasoji.denloye@intel.com> * - Modifies iaa behavior so that when QPL_STS_MORE_OUTPUT_NEEDED is returned eos isnt erronously set. Signed-off-by: Olasoji <olasoji.denloye@intel.com> * -Adds proper formatting Signed-off-by: Olasoji <olasoji.denloye@intel.com> * logging: add LOG_DEBUG level; qat: map log level to QATzip log level Introduce LOG_DEBUG (value=1) as the most verbose log level, shifting LOG_INFO to 2 and LOG_ERROR to 3. This follows the conventional ordering used by syslog, Log4j, Python logging, and QATzip itself (where higher values mean more verbose). The default log level remains LOG_ERROR (now value=3), so existing deployments see no behavioral change unless they explicitly set a lower value in /etc/zlib-accel.conf. The config parser max for log_level is updated from 2 to 3 accordingly. In qat.cpp, replace the hardcoded qzSetLogLevel(LOG_NONE) with a dynamic mapping from the configured log_level: LOG_DEBUG -> LOG_DEBUG3 (maximum QATzip verbosity) LOG_INFO -> LOG_INFO LOG_ERROR -> LOG_ERROR default -> LOG_NONE This corrects an inversion present in PR #55 of intel/zlib-accel, where LOG_ERROR was incorrectly mapped to LOG_DEBUG3. New tests: LogDebugLevel, LogDebugFilteredByInfo; LogLevelFiltering extended to cover DEBUG filtering. Builds on: #55 Signed-off-by: Olasoji <olasoji.denloye@intel.com> * Update README.md Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * qat: gate qzSetLogLevel mapping behind DEBUG_LOG; apply formatting Without the DEBUG_LOG guard, a release build (DEBUG_LOG=OFF) with a non-zero log_level in the config would cause QATzip to emit verbose output, contradicting the documented behaviour that log_level only applies when built with DEBUG_LOG=ON. Gate the dynamic mapping behind #ifdef DEBUG_LOG and unconditionally set LOG_NONE in the else branch, matching the original behaviour for non-debug builds. Addresses Copilot review comment on PR #56. Signed-off-by: Olasoji <olasoji.denloye@intel.com> * address review comments: enum ordering, call_once, explicit LOG_NONE case - logging.h: reorder LogLevel enum to match QATzip's QzLogLevel_T convention {LOG_NONE=0, LOG_ERROR=1, LOG_INFO=2, LOG_DEBUG=3}; flip filter direction from 'level < config' to 'level > config' so higher values = more verbose. Apply same fix to PrintDeflateBlockHeader. - qat.cpp: call qzSetLogLevel once per process via std::call_once instead of per session; add explicit case LogLevel::LOG_NONE; move default to end with comment; restructure #ifdef to be inside the once lambda. - config/config.cpp, config/default_config: update default log_level from 3 to 1 (LOG_ERROR = errors only, matching previous error-only default). - README.md: update log_level documentation to describe new ordering. - tests/zlib_accel_test.cpp: update expected log_level default to 1. --------- Signed-off-by: Olasoji <olasoji.denloye@intel.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * igzip: rename deflateSetDictionary/inflateSetDictionary to IGZIP variants Rename deflateSetDictionary and inflateSetDictionary in igzip.cpp to DeflateSetDictionaryIGZIP and InflateSetDictionaryIGZIP to eliminate the duplicate symbol conflict with the ZEXPORT versions in zlib_accel.cpp. The functions are stubs for future dictionary support (pending performance validation). Add declarations to igzip.h for when they are wired in. * sharded_map: add missing #include <mutex> for std::unique_lock std::unique_lock is defined in <mutex>, not <shared_mutex>. On GCC 13+ (Ubuntu 24.04), <shared_mutex> transitively includes <bits/unique_lock.h> making this invisible. On GCC 11 (Ubuntu 22.04) the include is not pulled in and the build fails with 'std::unique_lock is not a member of std'. Add explicit <mutex> include for standard compliance. Reported by matt-welch in PR #61 review. --------- Signed-off-by: Olasoji <olasoji.denloye@intel.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…56) (#62) * Adds test to validate log_stats_samples config * Reduces excessive error logging in QAT mode * Bugfix for the log stream * Treats QPL_STS_MORE_OUTPUT_NEEDED as a non error condition for IAA * Fixed formatting * - Modifies iaa behavior so that when QPL_STS_MORE_OUTPUT_NEEDED is returned eos isnt erronously set. * -Adds proper formatting * logging: add LOG_DEBUG level; qat: map log level to QATzip log level Introduce LOG_DEBUG (value=1) as the most verbose log level, shifting LOG_INFO to 2 and LOG_ERROR to 3. This follows the conventional ordering used by syslog, Log4j, Python logging, and QATzip itself (where higher values mean more verbose). The default log level remains LOG_ERROR (now value=3), so existing deployments see no behavioral change unless they explicitly set a lower value in /etc/zlib-accel.conf. The config parser max for log_level is updated from 2 to 3 accordingly. In qat.cpp, replace the hardcoded qzSetLogLevel(LOG_NONE) with a dynamic mapping from the configured log_level: LOG_DEBUG -> LOG_DEBUG3 (maximum QATzip verbosity) LOG_INFO -> LOG_INFO LOG_ERROR -> LOG_ERROR default -> LOG_NONE This corrects an inversion present in PR #55 of intel/zlib-accel, where LOG_ERROR was incorrectly mapped to LOG_DEBUG3. New tests: LogDebugLevel, LogDebugFilteredByInfo; LogLevelFiltering extended to cover DEBUG filtering. Builds on: #55 * Update README.md * qat: gate qzSetLogLevel mapping behind DEBUG_LOG; apply formatting Without the DEBUG_LOG guard, a release build (DEBUG_LOG=OFF) with a non-zero log_level in the config would cause QATzip to emit verbose output, contradicting the documented behaviour that log_level only applies when built with DEBUG_LOG=ON. Gate the dynamic mapping behind #ifdef DEBUG_LOG and unconditionally set LOG_NONE in the else branch, matching the original behaviour for non-debug builds. Addresses Copilot review comment on PR #56. * address review comments: enum ordering, call_once, explicit LOG_NONE case - logging.h: reorder LogLevel enum to match QATzip's QzLogLevel_T convention {LOG_NONE=0, LOG_ERROR=1, LOG_INFO=2, LOG_DEBUG=3}; flip filter direction from 'level < config' to 'level > config' so higher values = more verbose. Apply same fix to PrintDeflateBlockHeader. - qat.cpp: call qzSetLogLevel once per process via std::call_once instead of per session; add explicit case LogLevel::LOG_NONE; move default to end with comment; restructure #ifdef to be inside the once lambda. - config/config.cpp, config/default_config: update default log_level from 3 to 1 (LOG_ERROR = errors only, matching previous error-only default). - README.md: update log_level documentation to describe new ordering. - tests/zlib_accel_test.cpp: update expected log_level default to 1. --------- Signed-off-by: Olasoji <olasoji.denloye@intel.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR brings an initial ISA-L/igzip integration baseline into the main review flow, adding IGZIP as an additional execution path and enabling it as an intermediate fallback between hardware accelerators (IAA/QAT) and software zlib.
Changes:
- Add IGZIP as a selectable execution path for
deflate/inflate, plus optional accelerator→IGZIP fallback controlled by config. - Introduce ISA-L (IGZIP) build integration (CMake option, version gating) and expose new config options/statistics counters.
- Expand the test suite substantially with IGZIP-focused regression tests and new parameter combinations.
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
zlib_accel.h |
Adds IGZIP to ExecutionPath enum for path reporting/testing. |
zlib_accel.cpp |
Integrates IGZIP into deflate/inflate routing, fallback logic, and stream lifecycle handling. |
igzip.h |
Declares the IGZIP shim API used by zlib_accel.cpp. |
igzip.cpp |
Implements ISA-L-backed compress/uncompress helpers and stream handling. |
config/default_config |
Adds default toggles for IGZIP enablement. |
config/config.h |
Adds config enums for IGZIP enablement and fallback behavior. |
config/config.cpp |
Wires parsing/defaults for IGZIP-related configuration options. |
statistics.h |
Adds IGZIP counters for deflate/inflate success/error stats. |
statistics.cpp |
Adds IGZIP stat name strings to match new counters. |
tests/zlib_accel_test.cpp |
Adds IGZIP coverage across parametrized tests + large regression test suite. |
tests/CMakeLists.txt |
Minor update to run target command (commented filter). |
README.md |
Documents new CMake options, ISA-L dependency, and igzip_fallback; updates IAA marker docs. |
iaa.cpp |
Updates comments and changes IAA decompressibility heuristics (512-byte gate) tied to new fallback approach. |
sharded_map.h |
Adds missing <mutex> include (supports locking types used in the header). |
common.cmake |
Adds USE_IGZIP, ISA-L version enforcement, and ISA-L link/include setup. |
CMakeLists.txt |
Adds igzip.cpp to the shared library build. |
.gitignore |
Updates ignored build/artifact paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- zlib_accel.cpp deflateEnd: add null check before dereferencing deflate_settings (ShardedMap::Get returns nullptr for unregistered or double-ended streams) - zlib_accel.cpp inflateEnd: same null check for inflate_settings - zlib_accel.cpp inflateReset: move isal_strm check inside the inflate_settings null guard (was unconditionally dereferencing) - igzip.cpp CompressIGZIP: fix ret = 0:1 -> Z_OK:Z_STREAM_ERROR (1 == Z_STREAM_END, not an error code) - igzip.cpp/h: remove DeflateSetDictionaryIGZIP and InflateSetDictionaryIGZIP — UB casts on strm->state; will be re-added with correct design when dictionary support is implemented - common.cmake: remove invalid PUBLIC keyword from all three link_directories() calls (QPL, ISAL, QATZIP)
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
matt-welch
left a comment
There was a problem hiding this comment.
Inline comments left where appropriate.
PR #53 Review: IGzip Integration
Adds ISA-L (igzip) as a third accelerator backend, wired into deflate / inflate / compress2 / uncompress2 / gzwrite / gzread, with IAA/QAT → IGZIP fallback and ~1600 lines of regression tests. Substantial, well-structured work. Findings below are what to address before merge.
Correctness
1. UncompressIGZIP returns 1 instead of Z_DATA_ERROR on ISA-L errors : Medium
igzip.cpp:424 initializes ret = 1 and only reassigns it for ISAL_DECOMP_OK, ISAL_END_INPUT, and ISAL_NEED_DICT. Any other decomp code (e.g. ISAL_INVALID_BLOCK, ISAL_INVALID_SYMBOL, ISAL_INVALID_LOOKBACK) leaves ret == 1: not a valid zlib return code, and the log message reads "inflate finished with error code 1".
Downstream consequence: IGZIPRunInflateAndSelectPathAction at igzip.cpp:365-366 explicitly branches on *ret == Z_DATA_ERROR to return IGZIP_INFLATE_PATH_FALLBACK_DATA_ERROR. Because UncompressIGZIP never returns Z_DATA_ERROR from a real ISA-L error, that branch is only reachable via the null-arg guards at lines 339/349 : the intended "raw trailer fallback" path is effectively dead code.
Fix: map ISAL_INVALID_BLOCK/ISAL_INVALID_SYMBOL/ISAL_INVALID_LOOKBACK/default → Z_DATA_ERROR in the UncompressIGZIP return-code switch.
2. compress2 and uncompress2 use different completeness heuristics: Low
zlib_accel.cpp:926 checks ret == 0 && input_len != sourceLen to detect partial compression. zlib_accel.cpp:1028 mirrors this for uncompress2 but checks ret == 0 && !end_of_stream. Both paths are single-shot (no retry loop), matching the IAA/QAT stateless behavior, that's fine, but the two checks are testing different things for the same class of failure. Consider unifying or at least adding a comment.
3. Stateless compress2 / uncompress2 do not honor IGZIP_FALLBACK: Low
Streaming deflate() / inflate() have IAA/QAT → IGZIP fallback gated on configs[IGZIP_FALLBACK] (zlib_accel.cpp:421, :736). The stateless paths (zlib_accel.cpp:900-932, :998-1034) pick exactly one accelerator via if/else-if and never retry with IGZIP on failure. If IAA is preferred but fails, the caller falls straight to zlib, silently skipping IGZIP even when IGZIP_FALLBACK=1. Either wire fallback into the stateless paths or document the divergence.
4. gzread / gzwrite reverse the accelerator preference: Low (pre-existing, but propagates)
compress2/uncompress2/deflate/inflate: IAA first, then QAT, then IGZIP.GzwriteAcceleratorCompress(zlib_accel.cpp:1289) /GzreadAcceleratorUncompress(zlib_accel.cpp:1363): QAT first, then IAA, then IGZIP.
The QAT-first ordering pre-dates this PR (git blame confirms), but the PR wires IGZIP in as a third-choice fallback in both orderings without noting the inconsistency. Worth documenting or unifying.
Dead / Unused Code
5. crc32 / adler32 are dead code: Low (delete)
igzip.cpp:265-272 defines crc32() and adler32() at global scope, routed to ISA-L equivalents.
- Nothing in the project calls either function (grep-verified).
- They are not exported: the project builds with
-fvisibility=hidden(common.cmake:82), and onlyzlib_accel.hhas#pragma GCC visibility push(default).nm -Don the built.soshows nocrc32/adler32in the dynamic symbol table. zlib_accel.cppdoes not loadcrc32/adler32viadlsym: there are noorig_crc32/orig_adler32pointers.
So there is no LD_PRELOAD shadowing hazard, but the functions are pure dead code. Delete them and the #include "crc.h" at igzip.cpp:11.
6. Unused struct typedefs in igzip.h: Low
igzip.h:10-15:internal_state2/inflate_state2are declared but never referenced anywhere.igzip.h:17-22:deflate_stateis declared but never referenced. Also,deflate_stateis a well-known zlib-internal name; even if it were used, the collision would be worth avoiding.
Delete both.
7. Dead method = 0 assignments: Low
zlib_accel.cpp:401 and :428 set deflate_settings->method = 0 before calling InitCompressIGZIP. DeflateSettings::method is written in the constructor (line 190) and at these two sites, and read nowhere. Remove.
8. Dead parameter assignments in CompressIGZIP / UncompressIGZIP: Low
igzip.cpp:193-194:
input = isal_strm->next_in;
output = isal_strm->next_out;and igzip.cpp:417-418:
input = isal_strm_inflate->next_in;
output = isal_strm_inflate->next_out;Both assign to by-value function parameters that are never read after. Dead stores: delete.
9. IGZIPHandleActiveStreamNoInput return IGZIP_NO_INPUT_NOT_HANDLED is unreachable: Low
igzip.cpp:301 returns IGZIP_NO_INPUT_NOT_HANDLED only when strm->avail_in != 0 or on null args. The sole caller (zlib_accel.cpp:606) already guards strm->avail_in == 0 before calling, so that return value is effectively dead. Either simplify the function to bool / void or drop the caller guard.
10. Commented-out #define in header: Low
igzip.h:61: // #define Z_DEFAULT_COMPRESSION 6. Delete.
Style / Consistency
11. __LINE__ in log messages: Low
Log calls throughout igzip.cpp include __LINE__ as a positional argument, e.g. Log(..., "InitCompressIGZIP() Line ", __LINE__, ...). iaa.cpp, qat.cpp, and most of zlib_accel.cpp don't do this. Line numbers rot with every edit and the logs are noisier than the rest of the codebase. Match the existing style.
12. Hardcoded windowBits = 15 / 31 in stateless paths: Low
zlib_accel.cpp:917, :1018, :1316, :1390 (compress2, uncompress2, gzip accelerator helpers) pass bare 15 or 31 to InitCompressIGZIP / InitUncompressIGZIP. Values are correct (15 = zlib format, 31 = gzip format), but they should be named constants. Note: the same magic numbers already appear in the IAA/QAT branches (SupportedOptionsIAA(15, ...), CompressQAT(..., 31, ...), etc.): if cleaned up, do all three backends in one pass.
Test Coverage
- IGZIP tests are gated
#ifdef USE_IGZIPand CI does not build withUSE_IGZIP=ON(.github/workflows/*matrix builds the zlib-only path). Consider adding a CI job stub for when hardware/library is available, or at least a README note. compress2/uncompress2IGZIP paths are covered:ZlibCompressUtility2/ZlibUncompressUtility2(tests/zlib_accel_test.cpp:153-189) wrap them and are exercised by the parameterized matrix with IGZIP as one of the paths (tests/zlib_accel_test.cpp:299,:333). No gap here.
Summary
| # | Severity | File:Line | Issue |
|---|---|---|---|
| 1 | Medium | igzip.cpp:424 |
Missing Z_DATA_ERROR mapping; makes raw-trailer fallback branch dead code |
| 2 | Low | zlib_accel.cpp:926, 1028 |
compress2 vs uncompress2 completeness checks diverge |
| 3 | Low | zlib_accel.cpp:900-932, 998-1034 |
Stateless paths ignore IGZIP_FALLBACK |
| 4 | Low | zlib_accel.cpp:1289, 1363 |
gz* paths reverse QAT/IAA preference vs other paths |
| 5 | Low | igzip.cpp:265-272, :11 |
crc32/adler32 and crc.h include are dead code |
| 6 | Low | igzip.h:10-15, :17-22 |
Unused inflate_state2 / deflate_state typedefs |
| 7 | Low | zlib_accel.cpp:401, :428 |
Dead deflate_settings->method = 0 writes |
| 8 | Low | igzip.cpp:193-194, :417-418 |
Dead parameter assignments |
| 9 | Low | igzip.cpp:301 / zlib_accel.cpp:606 |
IGZIP_NO_INPUT_NOT_HANDLED unreachable |
| 10 | Low | igzip.h:61 |
Commented-out #define |
| 11 | Low | igzip.cpp (throughout) |
__LINE__ log style diverges |
| 12 | Low | zlib_accel.cpp:917, :1018, :1316, :1390 |
Magic 15/31 for windowBits |
Blocking: #1 (silently disables the raw-trailer fallback). Everything else is cleanup and can be addressed in a follow-up if preferred.
| ret = CompressIGZIP(isal_strm, Z_FINISH, const_cast<uint8_t*>(source), | ||
| &input_len, dest, &output_len, &total_in, &total_out); | ||
| EndCompressIGZIP(isal_strm); | ||
| if (ret == 0 && input_len != sourceLen) { |
There was a problem hiding this comment.
compress2 and uncompress2 use different completeness heuristics — Low
zlib_accel.cpp:926 checks ret == 0 && input_len != sourceLen to detect partial compression. zlib_accel.cpp:1028 mirrors this for uncompress2 but checks ret == 0 && !end_of_stream. Both paths are single-shot (no retry loop), matching the IAA/QAT stateless behavior — that's fine — but the two checks are testing different things for the same class of failure. Consider unifying or at least adding a comment.
| *end_of_stream = (isal_strm_inflate->block_state == ISAL_BLOCK_FINISH); | ||
| } | ||
|
|
||
| int ret = 1; |
There was a problem hiding this comment.
UncompressIGZIP returns 1 instead of Z_DATA_ERROR on ISA-L errors: Medium
igzip.cpp:424 initializes ret = 1 and only reassigns it for ISAL_DECOMP_OK, ISAL_END_INPUT, and ISAL_NEED_DICT. Any other decomp code (e.g. ISAL_INVALID_BLOCK, ISAL_INVALID_SYMBOL, ISAL_INVALID_LOOKBACK) leaves ret == 1: not a valid zlib return code, and the log message reads "inflate finished with error code 1".
Downstream consequence: IGZIPRunInflateAndSelectPathAction at igzip.cpp:365-366 explicitly branches on *ret == Z_DATA_ERROR to return IGZIP_INFLATE_PATH_FALLBACK_DATA_ERROR. Because UncompressIGZIP never returns Z_DATA_ERROR from a real ISA-L error, that branch is only reachable via the null-arg guards at lines 339/349: the intended "raw trailer fallback" path is effectively dead code.
Fix: map ISAL_INVALID_BLOCK/ISAL_INVALID_SYMBOL/ISAL_INVALID_LOOKBACK/default → Z_DATA_ERROR in the UncompressIGZIP return-code switch.
| if (*ret == 0) { | ||
| strm->next_out += output_len; | ||
| strm->avail_out -= output_len; | ||
| strm->total_out += output_len; |
There was a problem hiding this comment.
IGZIPHandleActiveStreamNoInput potential total_out double-counting
igzip.cpp:316 does strm->total_out += output_len after UncompressIGZIP returns. However, UncompressIGZIP already sets isal_strm_inflate->total_out = *total_out before calling isal_inflate, and inflate() in zlib_accel.cpp also increments strm->total_out at zlib_accel.cpp:787 after IGZIPHandleActiveStreamNoInput returns. Verify these update paths are mutually exclusive and total_out is not double-counted.
| #include <igzip_lib.h> | ||
| #include <zlib.h> | ||
|
|
||
| typedef struct internal_state2 { |
There was a problem hiding this comment.
Unused struct typedefs in igzip.h — Low
igzip.h:10-15—internal_state2/inflate_state2are declared but never referenced anywhere.igzip.h:17-22—deflate_stateis declared but never referenced. Also,deflate_stateis a well-known zlib-internal name; even if it were used, the collision would be worth avoiding.
Delete both.
| #ifdef USE_IGZIP | ||
| // Accelerator->IGZIP fallback: if IAA or QAT failed and IGZIP is | ||
| // available, retry with IGZIP before falling through to software zlib. | ||
| if ((path_selected == IAA || path_selected == QAT) && ret != 0 && |
There was a problem hiding this comment.
Stateless compress2 / uncompress2 do not honor IGZIP_FALLBACK — Low
Streaming deflate() / inflate() have IAA/QAT → IGZIP fallback gated on configs[IGZIP_FALLBACK] (zlib_accel.cpp:421, :736). The stateless paths (zlib_accel.cpp:900-932, :998-1034) pick exactly one accelerator via if/else-if and never retry with IGZIP on failure. If IAA is preferred but fails, the caller falls straight to zlib, silently skipping IGZIP even when IGZIP_FALLBACK=1. Either wire fallback into the stateless paths or document the divergence.
| return Z_OK; | ||
| } | ||
|
|
||
| unsigned long crc32(unsigned long crc, const unsigned char *buf, |
There was a problem hiding this comment.
crc32 / adler32 are dead code: Low (delete)
igzip.cpp:265-272 defines crc32() and adler32() at global scope, routed to ISA-L equivalents.
- Nothing in the project calls either function (grep-verified).
- They are not exported: the project builds with
-fvisibility=hidden(common.cmake:82), and onlyzlib_accel.hhas#pragma GCC visibility push(default).nm -Don the built.soshows nocrc32/adler32in the dynamic symbol table. zlib_accel.cppdoes not loadcrc32/adler32viadlsym: there are noorig_crc32/orig_adler32pointers.
So there is no LD_PRELOAD shadowing hazard, but the functions are pure dead code. Delete them and the #include "crc.h" at igzip.cpp:11.
| } else if (path_selected == IGZIP) { | ||
| #ifdef USE_IGZIP | ||
| if (deflate_settings->isal_strm == nullptr) { | ||
| deflate_settings->method = 0; |
There was a problem hiding this comment.
Dead method = 0 assignments — Low
zlib_accel.cpp:401 and :428 set deflate_settings->method = 0 before calling InitCompressIGZIP. DeflateSettings::method is written in the constructor (line 190) and at these two sites, and read nowhere. Remove.
|
|
||
| *output_length = original_avail_out - isal_strm->avail_out; | ||
| *input_length = original_avail_in - isal_strm->avail_in; | ||
| input = isal_strm->next_in; |
There was a problem hiding this comment.
Dead parameter assignments in CompressIGZIP / UncompressIGZIP: Low
igzip.cpp:193-194:
input = isal_strm->next_in;
output = isal_strm->next_out;and igzip.cpp:417-418:
input = isal_strm_inflate->next_in;
output = isal_strm_inflate->next_out;Both assign to by-value function parameters that are never read after. Dead stores: delete.
|
|
||
| IGZIPNoInputAction IGZIPHandleActiveStreamNoInput( | ||
| z_streamp strm, struct inflate_state *isal_strm_inflate, int *ret) { | ||
| if (strm == nullptr || isal_strm_inflate == nullptr || ret == nullptr || |
There was a problem hiding this comment.
IGZIPHandleActiveStreamNoInput return IGZIP_NO_INPUT_NOT_HANDLED is unreachable: Low
igzip.cpp:301 returns IGZIP_NO_INPUT_NOT_HANDLED only when strm->avail_in != 0 or on null args. The sole caller (zlib_accel.cpp:606) already guards strm->avail_in == 0 before calling, so that return value is effectively dead. Either simplify the function to bool / void or drop the caller guard.
| const unsigned long *total_out, bool *end_of_stream); | ||
| int EndUncompressIGZIP(struct inflate_state *isal_strm_inflate); | ||
| int ResetUncompressIGZIP(struct inflate_state *isal_strm_inflate); | ||
| // #define Z_DEFAULT_COMPRESSION 6 |
There was a problem hiding this comment.
Commented-out #define in header — Low
igzip.h:61: // #define Z_DEFAULT_COMPRESSION 6. Delete.
Blocking fix: - UncompressIGZIP: change 'int ret = 1' default to Z_DATA_ERROR so that ISAL_INVALID_BLOCK/SYMBOL/LOOKBACK and any other ISA-L error code correctly maps to Z_DATA_ERROR, enabling the raw-trailer fallback path in IGZIPRunInflateAndSelectPathAction. Dead code removal: - igzip.cpp: remove dead crc32()/adler32() functions and #include "crc.h" (not exported, not called anywhere in the project) - igzip.h: remove unused internal_state2/inflate_state2/deflate_state typedefs - igzip.h: remove commented-out #define Z_DEFAULT_COMPRESSION 6 - zlib_accel.cpp: remove dead deflate_settings->method = 0 assignments (DeflateSettings::method is never read) - igzip.cpp: remove dead post-call param assignments in CompressIGZIP and UncompressIGZIP (by-value params not read after assignment) Style / consistency: - igzip.cpp: remove __LINE__ from all 38 log calls to match iaa.cpp/qat.cpp style Correctness documentation: - IGZIPHandleActiveStreamNoInput: drop redundant strm->avail_in != 0 guard (caller already ensures avail_in == 0); add comment explaining that the total_out update here and the main inflate() update block are mutually exclusive (early return prevents double-counting) - compress2 / uncompress2: add comments explaining the intentional divergence in completeness checks (input_len vs end_of_stream) and that stateless paths do not honor IGZIP_FALLBACK by design
Signed-off-by: Olasoji <olasoji.denloye@intel.com>
|
Looks like there are a few bugs that predate this PR. I'll open a separate PR to address them. |
There was a problem hiding this comment.
PR #53 Review Summary — HEAD e814856
Actionable punch list from the second-pass review, verified against the current tree. More per-finding detail is available if needed.
Must fix
(3) — compress2() returns Z_OK on truncated output · zlib_accel.cpp:929
The check input_len != sourceLen verifies only input consumption. If avail_out runs out while ISA-L emits the trailer, isal_deflate returns COMP_OK with all input consumed, so the check passes and compress2 reports success on a truncated stream. Mirror uncompress2()'s !end_of_stream guard:
if (ret == 0 && (input_len != sourceLen ||
!IsIGZIPDeflateFinished(isal_strm))) {
ret = 1;
}Empirically reproduced. 64 bytes of 'A' → correct compressed size 11 bytes; calling compress2() with destLen=10 returns Z_OK but the output fails uncompress() with Z_DATA_ERROR. Reproducer: IGZIPCompress2RegressionTest.Compress2ReturnsZOkForTruncatedOutput in tests/zlib_accel_test.cpp.
Should fix
(1) — gz->path = IGZIP set even when init failed · zlib_accel.cpp:1338, 1413
Move the assignment inside the success else branch. Second-pass review originally tagged this "Must fix"; downgraded here — gz->path is only consulted as != ZLIB in outer gzwrite/gzread logic, so the effect is a benign retry, not state corruption.
(2) — implicit null tolerance in deflate() IGZIP init · zlib_accel.cpp:401–413, 426–441
CompressIGZIP currently handles a null isal_strm in its own guard. Add an explicit skip to the zlib fallback path when InitCompressIGZIP() returns nullptr.
(4) — IGZIP_NO_INPUT_NOT_HANDLED is dead · igzip.cpp:278, igzip.h:17, caller zlib_accel.cpp:610
The only path returning this value is a null-arg guard whose preconditions the caller already ensures. Simplify IGZIPHandleActiveStreamNoInput to a bool return and drop the caller's if (action == IGZIP_NO_INPUT_RETURN) check.
(5) — unreachable Z_STREAM_END log in CompressIGZIP · igzip.cpp:200–202
ret is set to Z_OK (0) or Z_STREAM_ERROR (−2), never Z_STREAM_END (1). Delete the branch.
(6) — same unreachable branch in UncompressIGZIP · igzip.cpp:411–413
ret is 0, Z_NEED_DICT (2), or Z_DATA_ERROR (−3), never Z_STREAM_END (1). Delete the branch.
Nice to have
(8) — unused strm / reason params · zlib_accel.cpp:245–263
SetDeflatePath / SetInflatePath cast both parameters to (void). Either wire them into the log messages or drop from the signature.
(9) — magic 15 / 31 windowBits literals · 12 call sites in zlib_accel.cpp
Replace with named constants (kWindowBitsZlib = 15, kWindowBitsGzip = 31) across all three backends in one pass.
Skip
(7) — add SupportedOptionsIGZIP() predicate
Proposed as a stub returning true for symmetry with the IAA/QAT gates. A no-op stub adds no filtering; the existing explicit configs[USE_IGZIP_COMPRESS] check is clearer. Skip unless a real gate (min input size, unsupported-format detection) is defined.
Must fix: - compress2: check IsIGZIPDeflateFinished() before EndCompressIGZIP() to detect truncated output when destLen is too small for the ISA-L trailer; input_len==sourceLen alone is insufficient since ISA-L returns COMP_OK with all input consumed but stream not in ZSTATE_END - compress2/uncompress2: simplify compress2 completeness check to !IsIGZIPDeflateFinished (subsumes input_len!=sourceLen); both paths now use a single terminal-state guard, symmetric with uncompress2 end_of_stream Should fix: - deflate(): wrap CompressIGZIP calls in null guard after InitCompressIGZIP; makes fallthrough-to-zlib explicit on init failure (both direct IGZIP path and IAA/QAT->IGZIP fallback path) - IGZIPHandleActiveStreamNoInput: change return type to void, remove dead IGZIPNoInputAction enum; caller unconditionally returns after the call - CompressIGZIP: remove unreachable else-if(ret==Z_STREAM_END) log branch; ret is only ever Z_OK or Z_STREAM_ERROR - UncompressIGZIP: same — remove unreachable else-if(ret==Z_STREAM_END) branch; ret is Z_OK, Z_NEED_DICT, or Z_DATA_ERROR Note: gz->path=IGZIP inside-else fix (should-fix #1) was already addressed in the previous commit (1874a71); no change needed here. Test: IGZIPDeflateRegressionTest.Compress2MustDetectTruncatedOutput added; all 56 IGZIP/config/logging/sharded-map tests pass.
Remove unused strm/reason params from SetDeflatePath/SetInflatePath and update all call sites. Replace magic windowBits literals 15 and 31 with kWindowBitsZlib and kWindowBitsGzip.
Summary
Initial draft PR to establish igzip integration branch for ongoing IGZIP fixes and improvements before merge to main.
Scope (Initial Push)
Brings current igzip branch baseline into review visibility.
Includes integration of igzip shim logic as well as functional tests.
Serves as the collaboration branch for follow-up PRs targeting igzip.
Purpose
Provide a shared upstream branch for iterative development, validation, and review.
Tracking
References #46